Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reverseproxy: Add handle_response blocks to reverse_proxy (#3710) #4021

Merged

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Feb 16, 2021

Supersedes #3712, closes #2920, #3707, #3828

Adds a new handle_response subdirective to reverse_proxy, and also @<name> named response matchers to pair with it.

Example which might serve files that are otherwise not accessible based on some response header:

reverse_proxy localhost:8080 {
	@accel header X-Accel-Redirect *
	handle_response @accel {
		root * /path/to/private/files
		rewrite {http.response.header.X-Accel-Redirect}
		file_server
	}
}

The matcher is optional; having no matcher will match all responses. Any handle_response without a matcher will be sorted to the end just in case the user put them in a bad order.

This also supports an optional status code on the same line, to change the status code of the response. If using this syntax, then a block may not be defined, and the matcher is required. (Why would you ever want to

reverse_proxy localhost:8080 {
	@fooIsBad header Foo *
	handle_response @fooIsBad 500
}

We should also remember to document that a status code matcher like 4 matches 4xx, as per the caddyhttp.StatusCodeMatches() function. It's useful.

@frou
Copy link

frou commented Feb 16, 2021

Sorry for what's maybe an elementary question, but can this be used to add some nuance to what Caddy does when the server being reverse proxied to is completely down?

Caddy currently serves a blank page with status code 502 (Bad Gateway) in that scenario. I'd like to be able to serve a page with a readable message, rather than just a blank page.

@francislavoie
Copy link
Member Author

@frou I'm pretty sure you can use https://caddyserver.com/docs/caddyfile/directives/handle_errors for that.

The difference is who emits the error. If it's Caddy itself that emits it, then you use handle_errors, and if the backend emits it as a response, then you use handle_response. A 502 is usually Caddy saying it couldn't connect to the upstream... But nothing stops an upstream from responding with a 502 if it feels like it.

@amyspark
Copy link

👋 I've been sorely trying to get a shadow of Gitlab Pages/nginx's error_page 404 /404.html with a proxy. Will this MR let me preserve the status code, while replacing only the response body with the result of handle_response?

@francislavoie
Copy link
Member Author

Thanks for sparking the question @amyspark 😂

I realized that there was a piece lacking to make that work. The file_server handler would only write a 200 status code when writing the file. To make what you want work, I wrote this PR #4076 that should do it, i.e. something like this:

reverse_proxy localhost:8080 {
	@fourhundred status 4
	handle_response @fourhundred {
		root * /path/to/errors
		rewrite * /{http.reverse_proxy.status_code}.html
		file_server {
			status {http.reverse_proxy.status_code}
		}
	}
}

@amyspark
Copy link

Close enough! but what I need is something like this:

reverse_proxy localhost:8080 {
	@fourhundred status 4
	handle_response @fourhundred {
		rewrite * /{http.reverse_proxy.status_code}.html
		reverse_proxy localhost:8080 {http.reverse_proxy.status_code}
	}
}

(i.e. rewrite the petition, send it again upstream, but keep the returned status code from the original response's)

@francislavoie
Copy link
Member Author

In that case you'll just need to make sure your upstream writes the correct status. I don't think it's a good idea to modify reverse_proxy for that.

mholt pushed a commit to caddyserver/website that referenced this pull request Apr 16, 2021
Docs for the new directive in caddyserver/caddy#4034.

Also adding a bit in `handle_errors` mentioning that `reverse_proxy` doesn't trigger errors when a response has an "error" HTTP status; not sure how to word this clearly, cause `reverse_proxy` still can trigger errors if there's no upstream or whatever. We should probably add examples for that later, and augment this paragraph to mention `handle_response` once that's merged (caddyserver/caddy#4021)
@francislavoie francislavoie force-pushed the reverse_proxy-handle_response branch 2 times, most recently from b452815 to 8130097 Compare April 22, 2021 02:21
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just little things, I think. Thanks for this contribution. Sorry it took me a while to devote the focus to review it.

I wonder if we could keep the UnmarshalCaddyfile() method if we keep the Caddyfile token parsing in there, but then leave any extra setup of the handler (e.g. the hanlde_response routes) to a caller, such as parseCaddyfile that takes the Helper. (I could elaborate in Slack...)

modules/caddyhttp/reverseproxy/caddyfile.go Outdated Show resolved Hide resolved
modules/caddyhttp/reverseproxy/caddyfile.go Outdated Show resolved Hide resolved
modules/caddyhttp/reverseproxy/caddyfile.go Show resolved Hide resolved
@francislavoie francislavoie force-pushed the reverse_proxy-handle_response branch 4 times, most recently from 274f4cc to 58a178e Compare May 2, 2021 17:59
mholt
mholt previously approved these changes May 2, 2021
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work! It turned out just about as I had hoped. Yes it's a bummer the complete Caddyfile unmarshaling can't take place inside UnmarshalCaddyfile(), but I think maintaining that interface is important, and the workaround is pretty reasonable.

It might be worth adding to the godoc comment for UnmarshalCaddyfile() that an additional call to Finalize... is needed when a Helper is available.

@francislavoie francislavoie force-pushed the reverse_proxy-handle_response branch from 0c96f05 to 14eb708 Compare May 2, 2021 18:19
@mholt mholt merged commit e4a22de into caddyserver:master May 2, 2021
@mholt
Copy link
Member

mholt commented May 2, 2021

Great work. Thanks for your patience and persistence with this.

@mholt mholt removed the under review 🧐 Review is pending before merging label May 2, 2021
@francislavoie francislavoie deleted the reverse_proxy-handle_response branch May 2, 2021 18:39
@francislavoie
Copy link
Member Author

And thanks for @maxatome for doing the groundwork for this! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify status code of reverse proxy response
5 participants